-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(query-core): prevent state override when observer remount occurs with signal consumption #9580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(query-core): prevent state override when observer remount occurs with signal consumption #9580
Conversation
…with signal consumption
WalkthroughAdds two tests for observer churn and overlapping fetches; tags observer-removal cancellations with isObserverRemoval; changes query fetch-error handling to return cached data when an observer-removal cancellation occurs and cached data exists; adds isObserverRemoval to CancelledError and CancelOptions. No public method signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant O as Observer1
participant Q as Query
participant R as Retryer
participant O2 as Observer2
O->>Q: subscribe() / trigger fetch A
Q->>R: start(fetch A, signalA)
Note over O,Q: Observer1 unmounts (StrictMode churn)
O-->>Q: unsubscribe()
Q->>R: cancel({revert: true, isObserverRemoval: true})
O2->>Q: subscribe() / trigger fetch B
Q->>R: start(fetch B, signalB)
R-->>Q: throw CancelledError{revert:true,isObserverRemoval:true} (for A)
alt isObserverRemoval && observers exist
alt state.data defined
Q-->>O2: keep fetchStatus 'fetching' and return cached data
else
Q-->>O2: rethrow CancelledError
end
else
Q->>Q: revert to #revertState; set fetchStatus 'idle'
end
R-->>Q: success for B -> Q updates state to data from B
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
View your CI Pipeline Execution ↗ for commit a69008b
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9580 +/- ##
===========================================
+ Coverage 45.15% 59.25% +14.10%
===========================================
Files 208 137 -71
Lines 8323 5566 -2757
Branches 1886 1495 -391
===========================================
- Hits 3758 3298 -460
+ Misses 4118 1964 -2154
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/query-core/src/types.ts (1)
1329-1333
: Add brief JSDoc for the new CancelOptions.isObserverRemoval flagNice, the new flag threads intent through the cancel pipeline. Since CancelOptions is part of the public types, add a short JSDoc to clarify internal usage so consumers don’t rely on it.
export interface CancelOptions { revert?: boolean silent?: boolean + /** + * Internal: marks cancellation caused by observer removal to disambiguate revert behavior. + * Not intended for external usage. + * @internal + */ isObserverRemoval?: boolean }packages/query-core/src/query.ts (1)
556-573
: Avoid reverting to idle when a new active observer exists; refine the guardThe new guard prevents an async revert from overriding an in-flight fetch after a quick re-subscribe. This addresses the StrictMode remount issue.
To avoid edge cases where a disabled observer re-subscribes (enabled: false) and we still skip revert (leaving fetchStatus stuck at 'fetching' without an active fetch), consider checking for an active observer rather than just a non-empty list.
- if (error.isObserverRemoval && this.observers.length > 0) { + if (error.isObserverRemoval && this.isActive()) { if (this.state.data === undefined) { throw error } return this.state.data }Optionally, add a short comment explaining why revert is skipped to aid future maintainers.
packages/query-core/src/__tests__/query.test.tsx (1)
1195-1239
: Remove ts-expect-error by typing the mock; clarify intentThe test relies on destructuring signal to mark the abortSignal as consumed. You can avoid the ts-expect-error by typing the mock to QueryFunction and keep the test self-documenting.
- // @ts-expect-error This field has been added for troubleshooting purposes. Disable ts error for testing. - const queryFn = vi.fn(async ({ signal }) => { + const queryFn = vi.fn<QueryFunction<string, ReturnType<typeof queryKey>>>(async ({ signal }) => { await sleep(50) return 'data' })Additionally, add the missing type import at the top of the file:
// augment existing import type block from '..' import type { QueryCache, QueryFunctionContext, QueryFunction, // <- add this QueryKey, QueryObserverResult, } from '..'This keeps the test free of suppression comments and makes the "signal consumption" intent explicit.
If you'd like, I can also contribute a complementary test that covers the edge-case where a disabled observer re-subscribes to ensure we still revert correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/query-core/src/__tests__/query.test.tsx
(1 hunks)packages/query-core/src/query.ts
(2 hunks)packages/query-core/src/retryer.ts
(1 hunks)packages/query-core/src/types.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/query-core/src/retryer.ts (1)
packages/query-core/src/types.ts (1)
CancelOptions
(1329-1333)
packages/query-core/src/__tests__/query.test.tsx (1)
packages/query-core/src/utils.ts (1)
sleep
(355-359)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/query-core/src/retryer.ts (1)
58-67
: Propagate observer-removal context via CancelledErrorCapturing isObserverRemoval on CancelledError is a clean way to alter downstream revert behavior without changing existing call sites. Looks good.
packages/query-core/src/query.ts (1)
350-355
: Emit isObserverRemoval on cancel when signal was consumedGood call to annotate the cancel originating from observer removal. This enables correct differentiation in the catch path and prevents unintended state reverts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/query-core/src/__tests__/query.test.tsx (2)
1196-1237
: Great regression test; consider asserting the cancellation type and avoiding microtask timing flakinessThis test reliably reproduces the StrictMode remount race and validates the intended behavior. Two small improvements:
- Assert the first fetch rejects with CancelledError (parity with the second test).
- Replace the microtask flush with waitFor to avoid timing flakiness tied to async cancel scheduling.
Apply this diff:
- await promise1.catch(() => {}) - - await Promise.resolve() + await expect(promise1).rejects.toBeInstanceOf(CancelledError) + await vi.waitFor(() => + expect(query.state.fetchStatus).toBe('fetching'), + )
1199-1203
: Minor: add a clarifying comment about signal destructuringDestructuring
signal
intentionally consumes the AbortSignal getter to set#abortSignalConsumed = true
. A short comment will make the intent obvious for future readers.- const queryFn = vi.fn(async ({ signal: _signal }) => { + const queryFn = vi.fn(async ({ signal: _signal }) => { + // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path await sleep(50) return 'data' })packages/query-core/src/query.ts (1)
556-573
: Correctly avoid revert when cancellation stems from observer removal and observers are activeThe guard
if (error.isObserverRemoval && this.isActive())
is the key to preventing the premature flip to 'idle'. Behavior looks correct:
- With active observers, skip revert. If there’s no cached data, rethrow to let the caller handle cancellation. If cached data exists, resolve with it without altering
fetchStatus
, preserving the in-progress state for the next fetch.- Otherwise, revert state and set
fetchStatus: 'idle'
(previous semantics).Two notes:
- Using
isActive()
(instead of justobservers.length > 0
) is more precise: it avoids skipping revert when only disabled observers are present and no new fetch will start. Good call.- Consider adding a brief comment explaining why we skip the revert here (to prevent the first fetch’s async cancellation from overriding the state after a new observer triggers a second fetch), for future maintainers.
Suggested inline comment:
- if (error.isObserverRemoval && this.isActive()) { + // If cancellation was caused by observer removal and there are active observers again, + // do not revert to idle: a new fetch may already be in flight, and reverting would + // incorrectly flip isLoading/isFetching to false under StrictMode remounts. + if (error.isObserverRemoval && this.isActive()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/query-core/src/__tests__/query.test.tsx
(1 hunks)packages/query-core/src/query.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/query-core/src/__tests__/query.test.tsx (1)
1239-1280
: Strong coverage for “no data” path; semantics match the fixThis test exercises the case where the first fetch is cancelled with revert (due to observer removal), no cached data exists, and a new observer immediately triggers a fresh fetch. It correctly asserts:
- The first promise rejects with CancelledError.
- fetchStatus remains 'fetching'.
- New data resolves as expected.
Looks good.
packages/query-core/src/query.ts (2)
347-356
: Tag observer-removal cancels to disambiguate revert behaviorPassing
isObserverRemoval: true
alongsiderevert: true
is the right lever to differentiate StrictMode cleanup from explicit user cancel. This enables fetch’s catch to avoid reverting state when a new observer is already active.
240-244
: All flags and types verified
CancelOptions
(packages/query-core/src/types.ts:1332) now declaresisObserverRemoval?: boolean
.CancelledError
(packages/query-core/src/retryer.ts:61–66) declaresisObserverRemoval?: boolean
and assigns it in the constructor.- The internal
retryer.cancel({ revert: true })
call inpackages/query-core/src/query.ts
(line 352) correctly passesisObserverRemoval: true
.No further changes needed.
It seems that an error unrelated to this PR is occurring at the CI stage. |
Not sure about this fix. I'll have to think a bit about it but I'm on vacation now. |
fixes: #9579
comment: #9579 (comment)
Description
This PR fixes an issue where
isLoading
incorrectly becomesfalse
during an active fetch when using React StrictMode with signal destructuring in the queryFn.Problem
This issue does not appear to be solely related to
StrictMode
, but rather occurs when usingsignal
within thequeryFn
underStrictMode
.In fact, if you remove the
signal
from the provided code as shown below,isLoading
will correctly becometrue
.The problem arises because when the
signal
option is present and the fetch starts, destructuring the signal setsabortSignalConsumed = true
query/packages/query-core/src/query.ts
Line 428 in 7c464e3
StrictMode
, during cleanup the following condition is met insideremoveObserver
query/packages/query-core/src/query.ts
Lines 351 to 352 in 7c464e3
This triggers an asynchronous execution of
this.#retryer.cancel({ revert: true })
.As a result, the sequence becomes: immediate re-mount → new observer added → second fetch begins → meanwhile, the asynchronous
catch
logic from the first fetch executes, which then hits the following branchquery/packages/query-core/src/query.ts
Lines 555 to 567 in 7c464e3
This causes the state to be overwritten with the initial values, leading to the observed issue.
The reason it did not occur in the previous version is that
onError
existed and handled the case, but now the logic is fully asynchronous and falls intocatch
, which results in the problem you are seeing.Solution
The fix adds a check to prevent state reversion when
isObserverRemoval
flag)this.observers.length > 0
)This pattern is typical of React StrictMode's cleanup simulation where components are immediately remounted.
Changes
isObserverRemoval
flag toCancelOptions
andCancelledError
removeObserver
to pass this flag when cancelling with revertfetch
method to check for this condition before reverting stateTesting
should not override fetching state when revert happens after new observer subscribes
Summary by CodeRabbit
Bug Fixes
Types
Tests